Channel Establishment for V3 Channels #3792
Conversation
|
👋 Thanks for assigning @tankyleo as a reviewer! |
| @@ -16153,22 +16154,28 @@ mod tests { | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_anchors_zero_fee_htlc_tx_fallback() { | |||
| fn test_anchors_zero_fee_htlc_tx_downgrade() { | |||
There was a problem hiding this comment.
Congrats, you touched it, now you get to move it out of channelmanager into some other test-specific file that isn't 15000 lines of code 😂
There was a problem hiding this comment.
Done this in a follow up in #3797 so that move + format can be reviewed separately.
lightning/src/util/config.rs
Outdated
| /// back to a `anchors_zero_fee_htlc` (if [`Self::negotiate_anchors_zero_fee_htlc_tx`] | ||
| /// is set) or `static_remote_key` channel. | ||
| /// | ||
| /// *Implies [`Self::negotiate_anchors_zero_fee_htlc_tx`].* |
There was a problem hiding this comment.
These are in conflict - one says we'll fall back if its set, the other says that its implied (ie always set) if we set this flag.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
|
✅ Added second reviewer: @valentinewallace |
carlaKC
force-pushed
the
3789-channelestablishment
branch
from
3a36506 to
b9ec89c
Compare
|
Removed conflicting docs statement + opened followup for test separation (felt wrong to do move+format in the same PR, happy to include if we want it in here). |
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
carlaKC
force-pushed
the
3789-channelestablishment
branch
from
b9ec89c to
892d87a
Compare
|
Major change in push is using get_initial_channel_type in channel type downgrades to DRY up the code a bit. |
| // fees, because we downgrade from this channel type first. If there were a superior | ||
| // channel type that downgrades to `anchor_zero_fee_commitments`, we'd need to handle | ||
| // fee setting differently here. | ||
| assert!(!next_channel_type.supports_anchor_zero_fee_commitments()); |
There was a problem hiding this comment.
A note: I think Matt mentioned in the past to avoid hard asserts unless we are at risk of losing funds (and instead favor debug asserts). This assert makes sense to me though :)
There was a problem hiding this comment.
Good to know! Not familiar w/ the conventions for the project.
Will leave this as-is pending input from a second reviewer.
There was a problem hiding this comment.
Indeed, if we think we can lose funds we should hard-assert, if we don't, we should debug-assert. If we get 0FC here we'd presumably set a fee, and then I assume that implies we'd be vulnerable to funds loss (as the commitment would be tagged TRUC but not 0F and might have a zero-value anchor, making it ineligible for relay under policy)? If that's the theory, makes sense, but might be worth highlighting the risks a bit clearer in the comment.
There was a problem hiding this comment.
Indeed, if we got next_channel_type.supports_anchor_zero_fee_commitments here, we'd try to open a 0F channel with non-zero fees.
We'd probably just fail to open the channel because our peer would reject it, but probably don't want to rely on that (something, something, attacker lets us create this unrelayable commitment and then uses an accelerator?).
Will leave assert and update the comment 👍
carlaKC
force-pushed
the
3789-channelestablishment
branch
from
892d87a to
e56e32a
Compare
|
🔔 3rd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 3rd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 5th Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
| // fees, because we downgrade from this channel type first. If there were a superior | ||
| // channel type that downgrades to `anchor_zero_fee_commitments`, we'd need to handle | ||
| // fee setting differently here. | ||
| assert!(!next_channel_type.supports_anchor_zero_fee_commitments()); |
There was a problem hiding this comment.
Indeed, if we think we can lose funds we should hard-assert, if we don't, we should debug-assert. If we get 0FC here we'd presumably set a fee, and then I assume that implies we'd be vulnerable to funds loss (as the commitment would be tagged TRUC but not 0F and might have a zero-value anchor, making it ineligible for relay under policy)? If that's the theory, makes sense, but might be worth highlighting the risks a bit clearer in the comment.
carlaKC
force-pushed
the
3789-channelestablishment
branch
from
e56e32a to
5b568c3
Compare
|
Rebased + added additional comment on assert - diff. |
|
Two followups for this to do some test cleaning while we're here:
|
| eligible_features.clear_anchors_zero_fee_htlc_tx(); | ||
| eligible_features.clear_anchor_zero_fee_commitments(); | ||
| assert!(!eligible_features.supports_scid_privacy()); | ||
| assert!(!eligible_features.supports_anchors_nonzero_fee_htlc_tx()); |
There was a problem hiding this comment.
should have raised this nit earlier sorry: for consistency shouldn't we also add this here and in the previous branch ?
assert!(!eligible_features.supports_anchors_zero_fee_htlc_tx());
Also before this commit, we were just sanity checking that we aren't supporting the deprecated nonzero_fee_htlc_tx type, but now we also check that the earlier clear statements worked correctly ?
There was a problem hiding this comment.
assert!(!eligible_features.supports_anchors_zero_fee_htlc_tx());
Nice catch!
Also before this commit, we were just sanity checking that we aren't supporting the deprecated nonzero_fee_htlc_tx type, but now we also check that the earlier clear statements worked correctly ?
I was worried about not clearing out the feature bits in the correct order and ending up in a weird loop where we downgrade to one channel type, don't clear the feature bits and then "downgrade" back to a type we already tried - which would mean we endlessly retry the channel. So we want to be really sure that we're clearing all of the features for our channel type and "greater".
Perhaps it's overkill/should be debug?
Useful for the commits that follow where we add more downgrade tests.
carlaKC
force-pushed
the
3789-channelestablishment
branch
from
5b568c3 to
e430212
Compare
Rather than duplicating our channel type preference ordering in downgrade logic, make a modified version of the remote peer's supported features and remove our current channel type from it to get the next preferred channel type.
To allow testing along the way in this PR, turn on negotiation of zero fee channels. Co-authored-by: Matt Corallo <[email protected]>
Sender: MUST set `feerate_per_kw` to zero Receiver: MUST fail the channel if `feerate_per_kw` != 0 Co-authored-by: Matt Corallo <[email protected]>
Like anchor channels, these channels require that the user reserves a UTXO to bump the channel. If we automatically accept this channel type and the user does not have such reserve available, they are at risk of losing funds because they cannot fee bump the channel.
carlaKC
force-pushed
the
3789-channelestablishment
branch
from
e430212 to
99c645e
Compare
This PR updates the channel establishment flow to allow and validate V3 channels (behind
testflag).